Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[8팀 조우영] [Chapter 1-2] 프레임워크 없이 SPA 만들기 #27

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

yeong30
Copy link

@yeong30 yeong30 commented Dec 26, 2024

과제 체크포인트

기본과제

가상돔을 기반으로 렌더링하기

  • createVNode 함수를 이용하여 vNode를 만든다.
  • normalizeVNode 함수를 이용하여 vNode를 정규화한다.
  • createElement 함수를 이용하여 vNode를 실제 DOM으로 만든다.
  • 결과적으로, JSX를 실제 DOM으로 변환할 수 있도록 만들었다.

이벤트 위임

  • 노드를 생성할 때 이벤트를 직접 등록하는게 아니라 이벤트 위임 방식으로 등록해야 한다
  • 동적으로 추가된 요소에도 이벤트가 정상적으로 작동해야 한다
  • 이벤트 핸들러가 제거되면 더 이상 호출되지 않아야 한다

심화 과제

1) Diff 알고리즘 구현

  • 초기 렌더링이 올바르게 수행되어야 한다
  • diff 알고리즘을 통해 변경된 부분만 업데이트해야 한다
  • 새로운 요소를 추가하고 불필요한 요소를 제거해야 한다
  • 요소의 속성만 변경되었을 때 요소를 재사용해야 한다
  • 요소의 타입이 변경되었을 때 새로운 요소를 생성해야 한다

2) 포스트 추가/좋아요 기능 구현

  • 비사용자는 포스트 작성 폼이 보이지 않는다
  • 비사용자는 포스트에 좋아요를 클릭할 경우, 경고 메세지가 발생한다.
  • 사용자는 포스트 작성 폼이 보인다.
  • 사용자는 포스트를 추가할 수 있다.
  • 사용자는 포스트에 좋아요를 클릭할 경우, 좋아요가 토글된다.

과제 셀프회고

기술적 성장

  • 가상 DOM의 동작 원리에 대해 조금 막연하게 알고 있었는데, 어떠한 비교과정을 통하는지 이번 과제를 통해 명확하게 이해할 수 있었습니다.
  • 또한 이벤트 위임을 어떠한 방식으로 활용 방법과 그 효율성에 대해 많이 배웠습니다.

코드 품질

  • 중복되는 로직은 util함수로 빼었지만, 명시적으로 과정을 나열한 코드보다 조금 더 선언형 프로그래밍 방식으로 개선해보고 싶습니다.

리뷰 받고 싶은 내용

Copy link

@devJayve devJayve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

우영님 간단하게 리뷰 남겨놓았는데 확인 부탁드립니다~! 전반적으로 코드 흐름이 깔끔하고 디테일한 부분(ex. 커스텀 에러 객체, 유틸 함수, 인덱스 접근 시 length 고려 등..)을 잘 챙긴 것 같아 리뷰하면서 저도 많이 배웠습니다 !! 의견 하나만 말씀드리자면 핵심적인 부분(createElement, normalizeVNode 등)에 흐름을 파악할 수 있는 주석을 간단히 남겨놓으신다면 더 좋을 것 같아요! 👍👍

Comment on lines +23 to +30
resolve: {
alias: [
{
find: "@",
replacement: "/src",
},
],
},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 이렇게 선언하면 파일 위치가 바뀌거나 복잡한 상대경로를 좀 더 깔끔하게 선언할 수 있겠네요!! 디테일한 부분 참고하겠습니다
그런데 @로 별칭을 지정했는데 사용된 부분이 없어 이 부분도 적용하면 좋을 것 같습니다

// 기존 코드
import { getEventTypeFromProps } from "../utils/eventUtils"; 

// 수정 코드
import { getEventTypeFromProps } from "@/utils/eventUtils";

Comment on lines +1 to +7
export class InvalidVNodeTypeError extends Error {
static MESSAGE = "InvalidVNodeTypeError";

constructor() {
super(InvalidVNodeTypeError.MESSAGE);
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 유효하지 않은 노드에 대해 에러를 처리할 때 커스텀한 에러를 만들어볼까.. 하다가 까먹었는데 이런 디테일한 부분들을 잘 신경써서 코딩하시는 습관이 부럽습니다😂 저도 참고하고 배우겠습니다..

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 배우고 갑니당..!👍

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

배우고 갑니다.!!

Comment on lines 24 to 27
<span
onClick={onToggleLike.bind(null, id)}
className={`like-button cursor-pointer${activationLike ? " text-blue-500" : ""}`}
>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 이렇게 함수에 파라미터를 바인딩하는 방식을 사용한 이유가 궁금해요! 클로저 함수로 생성하지 않고 globalAction에 있는 onToggleLike를 바인딩해서 함수의 재생성이나 변경을 방지하는 용도인가요 ..??
(맞다면 실제 의도한 바대로 잘 동작했는지도 궁금합니다!!)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 저도 명시적인 바인딩을 사용하신 이유가 궁금해요!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵! 사실 props에 인라인 함수를 지향하자는 컨벤션에서 비롯된 습관?입니다. 말씀해주신대로 인라인의 경우 클릭 시 마다 함수가 재생성되는 문제를 방지할 수 있고요! 다만 정말 작은 차이이기 때문에� 성능보다는 개인적인 컨벤션이라서 해당 방식을 사용했습니다.
저도 궁금 해서 gpt에서 확인해보니
성능 최적화가 필요하지 않은 상황에서는 onClick={() => onToggleLike(id)}를 사용하는 것이 일반적입니다.
라고 하네요,,

Comment on lines +5 to +6
const targetgetEvent = events[e.type].get(e.target);
if (targetgetEvent) targetgetEvent(e);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

targetgetEvent라는 변수명보단 targetHandler, targetEvent 혹은 eventHandler와 같은 변수명을 고려해봐도 좋을 것 같아요!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 도운님과 동일한 생각인데요~!
변수/함수명을 지을때 동사+명사 조합으로 지어주시는게 좋습니다 :)
도운님의 예시를 참고해주시면 좋을 것 같아요!

src/lib/createElement.js Show resolved Hide resolved
Comment on lines 20 to 21
const vNodoes = vNode.map(normalizeVNode);
return vNodoes;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오타인 것 같은데 vNodes로 수정하면 좋을 것 같습니다~

Suggested change
const vNodoes = vNode.map(normalizeVNode);
return vNodoes;
const vNodes = vNode.map(normalizeVNode);
return vNodes;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 감사합니다! 급하게 하느라 오타가 많이 심각하네요..

const handleSubmitPostForm = (e) => {
e.preventDefault();

const content = document.querySelector("#post-content").value;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

querySelector로 ID를 하드코딩하여 접근하는 방식보다 event 객체(e)의 target을 이용하여 content에 접근하는 방식을 고려해보면 어떨까요?? 개인적으로는 더 안정적이고 코드 변동 시(ex. id 값의 변동) 대응이 더 쉬울 것 같은데 우영님의 의견도 궁금합니다 !!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그렇게 접근하는 방법도 있었군요.. 감사합니다!

Comment on lines 24 to 27
<span
onClick={onToggleLike.bind(null, id)}
className={`like-button cursor-pointer${activationLike ? " text-blue-500" : ""}`}
>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 저도 명시적인 바인딩을 사용하신 이유가 궁금해요!

@@ -20,12 +28,19 @@ export const HomePage = () => {
<Navigation />

<main className="p-4">
<PostForm />
{loggedIn && <PostForm onSubmit={handleSubmitPostForm} />}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

혹시 form에서만 사용하는 이벤트라면 PostForm 컴포넌트에 선언할 수도 있었을 것 같은데 props로 전달하신 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

보통 하위 컴포넌트를 분리할 때, 컴포넌트 재사용성을 높이기 위해 비지니스 로직은 외부 핸들러 방식을 선호해서 다음과 같이 사용했습니다! 현재는 단순 add만 해주지만, 페이지에 따라 추가적인 처리를 해야하는 경우도 있으니까요.
다만, 원정님 피드백을 받고 생각해보니 해당 프로젝트는 PostForm이 재사용성이 없으니 내부 핸들러 방식이 더 적합한거 같습니다! 외부 핸들러를 사용하려면 아예 Form 컴포넌트로 분리 후 사용하는게 더 적합하겠어요ㅎㅎ 감사합니다!

Comment on lines +1 to +7
export class InvalidVNodeTypeError extends Error {
static MESSAGE = "InvalidVNodeTypeError";

constructor() {
super(InvalidVNodeTypeError.MESSAGE);
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 배우고 갑니당..!👍

Comment on lines +7 to +9
if (typeof vNode === "function") {
throw new InvalidVNodeTypeError();
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다들 예외처리부분에서 디테일하게 잘 처리하시는 것 같습니다 bb!

Comment on lines +48 to +51
if (newNode.type === oldNode.type) {
updateAttributes(oldElement, newNode.props || {}, oldNode.props || {});
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

위에서 다를 때 조건을 이미 체크했기때문에 한번 더 체크하지 않아도 될것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이부분은 사실 위에서 반대케이스가 체크되긴 한데, 제가 어떤 케이스들이 걸리는지 알고싶어서 남겨두었었어요! 생각해보니 주석으로 남기는게 더 좋겠네요! 감사합니다

Copy link

@osohyun0224 osohyun0224 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 우영님~! 담당 학습메이트 오소현입니다 :)

우영님의 코드는 언제나 핵심만 딱딱 다양한 예외 처리 고려까지 너무 잘 작성해주신 것 같습니다 ㅎㅎ 앞서서 리뷰해주신 도운, 원정, 준만, 근백님의 리뷰를 많이 참고해주시고 다른 분들이 리뷰를 정말 잘해주셨습니다 bb 최고네요 저도 리뷰 보면서 열심히 배웠습니다 ㅎㅎ

항상 모범 코드 처럼 깔끔하게 작성해주시는 비결이 궁금합니다 ..👀 10주간 보면서 저도 우영님 코드보고 많이 배우겠습니다 :)

이번주도 과제하시느라 너무 고생많으셨습니다 ㅠㅠ 다음주 과제도 화이팅입니다 ~! 멘토링 사전 노트도 잘 챙겨주셔서 너무 감사했어요! :) 언제나 화이팅입니다 👍 👍

+PS. 바쁘시겠지만 ㅠㅠ PR을 조금 더 작성해주시면 좋을 것 같습니다 현재 리뷰받고 싶은 내용이 비어있어요..! 다음에는 더 잘챙겨주시면 코치님들의 피드백도 원하는 방향으로 잘 받으실 수 있지 않을까 생각합니다 :)

Comment on lines +3 to +7
export function createVNode(type, props, ...childrens) {
const flatChildren = childrens
.flat(Infinity)
.filter((children) => isRenderableVNode(children));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

props와 childrens에 대해 타입 체크가 있었으면 좋겠습니다 bb

Comment on lines +5 to +6
const targetgetEvent = events[e.type].get(e.target);
if (targetgetEvent) targetgetEvent(e);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 도운님과 동일한 생각인데요~!
변수/함수명을 지을때 동사+명사 조합으로 지어주시는게 좋습니다 :)
도운님의 예시를 참고해주시면 좋을 것 같아요!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants